services/horizon: Fix ReapLookupTables query #4525
Conversation
There was a problem hiding this comment.
I have a general question about the way reaping works, as that isn't part of the codebase I have had much exposure to yet.
The docstring on ReapLookupTables states that it
removes rows from lookup tables which aren't used (orphaned), i.e. history entries for them were reaped
To me, the bigger picture of reaping entries has to do with ledger advancement, right? i.e. you set HISTORY_RETENTION_COUNT=x means data from x ledgers get kept around. If every table either (a) tracks the ledger a row applies to or (b) can somehow be joined to identify said ledger, isn't reaping then just a matter of DELETE FROM table WHERE ledgerSeq < latestIngestedLedgerSeq; for every table?
In a related vein, it'd be nice if this PR clarified what offsets means within the docstring of ReapLookupTables. (To be clear, I'm aware of their purpose thanks to the awesome description in #4518, but adding it inline to the code will be helpful I think!)
| @@ -935,7 +957,7 @@ func constructReapLookupTablesQuery(table string, historyTables []tableObjectFie | |||
| for i, historyTable := range historyTables { | |||
| _, err = fmt.Fprintf( | |||
| &sb, | |||
| `(select count(*) from %s where %s = hcb.id) as c%d, `, | |||
| `(select 1 from %s where %s = hcb.id limit 1) as c%d, `, | |||
| "(select 1 from history_trades where base_account_id = hcb.id limit 1) as c2, "+ | ||
| "(select 1 from history_trades where counter_account_id = hcb.id limit 1) as c3, "+ | ||
| "(select 1 from history_transaction_participants where history_account_id = hcb.id limit 1) as c4, "+ | ||
| "1 as cx from history_accounts hcb where id >= 0 order by id limit 10) as sub "+ |
There was a problem hiding this comment.
No, the 0 in id >=0 condition is the offset param and it's there just to ensure we move forward when iterating over the tables in batches. So for next cycle it will be id >=1000 if the ID after 1000 rows (batch size) is equal 1000.
There was a problem hiding this comment.
Nit: Could probably clean this up with WITH clauses, and named sub-queries?
This is exactly what The obvious problem with lookup tables is that a given ID can be used in multiple ledger ranges (depends on activity of an account or claimable balance). So we can't just simply remove rows from lookup tables while removing rows from normal history tables. Second, lookup tables are actively used by ingestion so if reaper removes a lookup row after ingestion loaded it it can cause inconsistencies in the DB. |
This commit fixes the query added in ee063a7. The previous query was using
limit ... offset ...which becomes slow for largeroffsetvalues. This is happening because (from docs[1]):I rewrote the query to use
idfield in tables inwhere id > .... Before each run, the newidoffset is fetched from the table that is later returned to be used in the next cycle.I also realized that rewriting
count(*)in subqueries to1withlimit 1further improves the query performance. This is because instead of counting all rows in parent tables it just check if there are any rows in tables (what we need to determine if the row is orphaned or not). This allows running the reaper also forhistory_accountsandhistory_assetstables.[1] https://www.postgresql.org/docs/current/queries-limit.html